Fix #14334: Support more msbuild conditional constructs #8039

autoantwort · 2025-12-16T18:33:09Z

No description provided.

lib/importproject.cpp

firewave · 2025-12-16T20:38:01Z

Thanks for your contribution.

Please avoid adding test-only code in the production code. See e04019c for an approach. If you inherit from ProjectConfiguration the added function could be moved into the test code and the additional constructor and moved implementation could me made protected.

lib/importproject.cpp

firewave · 2025-12-16T20:43:44Z

lib/importproject.cpp

-            tokenlist.createAst();
-            for (const Token *tok = tokenlist.front(); tok; tok = tok->next()) {
-                if (tok->str() == "(" && tok->astOperand1() && tok->astOperand2()) {
-                    // TODO: this is wrong - it is Contains() not Equals()


We should have filed a ticket for this since fixing it is a behavior change. Are you able to do that or should I file one?

Could you do that? :)

Will do later. I want to have a reproducer first.

autoantwort · 2025-12-17T12:43:30Z

Please avoid adding test-only code in the production code. See e04019c for an approach. If you inherit from ProjectConfiguration the added function could be moved into the test code and the additional constructor and moved implementation could me made protected.

Currently ProjectConfiguration and ConditionalGroup are an implementation detail and defined in the cpp file. I would have to move the classes to the header. Then they become an public interface only because of test-only code.
My thought was that exposing a single test-only function is better than exposing two complete implementation details classes.

firewave · 2025-12-17T12:47:37Z

Currently ProjectConfiguration and ConditionalGroup are an implementation detail and defined in the cpp file.

Sorry that I missed that.

I would have to move the classes to the header. Then they become an public interface only because of test-only code.
My thought was that exposing a single test-only function is better than exposing two complete implementation details classes.

Let's get the CI to pass first and the other stuff addressed and then I will have another look. It won't be merged before we made the 2.19 release anyways.

lib/importproject.cpp

danmar · 2026-01-02T11:39:53Z

lib/importproject.cpp

+                if (startsHere(i, "$(Configuration."))
+                {
+                    initialized = true;
+                    if (startsHere(i, "Contains("))


is it possible to have spaces.. Contains ( ?

Never have seen this, but it seems to be possible.

I would assume that these expressions can be handwritten? So some spaces should be expected?

I already have added support here

lib/importproject.cpp

test/testimportproject.cpp

lib/importproject.cpp

autoantwort · 2026-01-04T01:36:20Z

The selfcheck failure seems like an cppcheck error:
With 4f9f852 I get the error

Run supprs="--suppress=unusedFunction:lib/errorlogger.h:196"
lib/importproject.cpp:1676:21: style: The function 'selectVsConfigurations' is never used. [unusedFunction]
void ImportProject::selectVsConfigurations(Platform::Type platform, const std::vector<std::string> &configurations)
                    ^
lib/importproject.cpp:1700:39: style: The function 'getVSConfigs' is never used. [unusedFunction]
std::list<std::string> ImportProject::getVSConfigs()
                                      ^

without I get

Run ./cppcheck -q --template=selfcheck --error-exitcode=1 --library=cppcheck-lib --library=qt -D__CPPCHECK__ -D__GNUC__ -DQT_VERSION=0x060000 -DQ_MOC_OUTPUT_REVISION=69 -DQT_CHARTS_LIB -DQT_MOC_HAS_STRINGDATA --enable=unusedFunction,information --exception-handling -rp=. --project=cmake.output.notest/compile_commands.json --suppressions-list=.selfcheck_unused_suppressions --inline-suppr
lib/importproject.cpp:1677:0: information: Unmatched suppression: unusedFunction [unmatchedSuppression]
void ImportProject::selectVsConfigurations(Platform::Type platform, const std::vector<std::string> &configurations)
^
lib/importproject.cpp:1702:0: information: Unmatched suppression: unusedFunction [unmatchedSuppression]
std::list<std::string> ImportProject::getVSConfigs()
^

sonarqubecloud · 2026-01-08T17:11:10Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

autoantwort · 2026-01-08T17:11:33Z

I only rebased with main

danmar

Why don't you tokenize as before? That would get rid of a lot of the string handling. And it is more flexible it will allow spaces between tokens. As far as I see your code requires for instance that there is no spaces in $(Configuration) .

danmar · 2026-01-08T20:21:42Z

lib/importproject.cpp

+                    throw std::runtime_error("Expecting a start of a string!");
+                }
+                auto start = ++i;
+                while (i < expr.length() && expr[i] != '\'') ++i;


Suggested change

while (i < expr.length() && expr[i] != '\'') ++i;

i = expr.find('\'', i);

danmar · 2026-01-08T20:23:35Z

lib/importproject.cpp

+                }
+                auto start = ++i;
+                while (i < expr.length() && expr[i] != '\'') ++i;
+                std::string string = expr.substr(start, i - start);


I feel that the string can be created below after checking if i is valid.

autoantwort · 2026-01-08T23:23:41Z

Why don't you tokenize as before? That would get rid of a lot of the string handling. And it is more flexible it will allow spaces between tokens. As far as I see your code requires for instance that there is no spaces in $(Configuration) .

Because I only know that it didn't worked. Does the tokenizer know how to tokenize in strings? Feel free do create a PR at my feature branch :)

github-advanced-security bot found potential problems Dec 16, 2025

View reviewed changes

lib/importproject.cpp Fixed Show fixed Hide fixed

autoantwort force-pushed the fix-14334 branch from a8ea143 to dade829 Compare December 16, 2025 18:43

firewave reviewed Dec 16, 2025

View reviewed changes

lib/importproject.cpp Outdated Show resolved Hide resolved

firewave reviewed Dec 16, 2025

View reviewed changes

firewave marked this pull request as draft December 16, 2025 20:44

autoantwort force-pushed the fix-14334 branch 7 times, most recently from a327364 to e674fb6 Compare December 17, 2025 17:57

autoantwort requested a review from firewave December 30, 2025 22:37

autoantwort marked this pull request as ready for review December 30, 2025 22:37

autoantwort force-pushed the fix-14334 branch 3 times, most recently from 0818e64 to f0810d5 Compare December 31, 2025 02:27

danmar reviewed Jan 2, 2026

View reviewed changes

autoantwort force-pushed the fix-14334 branch from f0810d5 to a16e00d Compare January 2, 2026 14:50

github-advanced-security bot found potential problems Jan 2, 2026

View reviewed changes

lib/importproject.cpp Fixed Show fixed Hide fixed

autoantwort force-pushed the fix-14334 branch 3 times, most recently from c1c4c5e to 35be150 Compare January 2, 2026 16:29

firewave reviewed Jan 2, 2026

View reviewed changes

lib/importproject.cpp Show resolved Hide resolved

firewave reviewed Jan 2, 2026

View reviewed changes

test/testimportproject.cpp Outdated Show resolved Hide resolved

firewave reviewed Jan 2, 2026

View reviewed changes

lib/importproject.cpp Show resolved Hide resolved

autoantwort force-pushed the fix-14334 branch from 35be150 to cda2362 Compare January 2, 2026 16:52

firewave reviewed Jan 2, 2026

View reviewed changes

lib/importproject.cpp Show resolved Hide resolved

autoantwort force-pushed the fix-14334 branch 3 times, most recently from bf81c63 to f9f9c64 Compare January 3, 2026 18:52

autoantwort force-pushed the fix-14334 branch from 4f9f852 to b4b0a1f Compare January 4, 2026 01:39

Fix #14334: Support more msbuild conditional constructs

738a14a

autoantwort force-pushed the fix-14334 branch from b4b0a1f to 738a14a Compare January 8, 2026 17:10

danmar reviewed Jan 8, 2026

View reviewed changes

	while (i < expr.length() && expr[i] != '\'') ++i;
	i = expr.find('\'', i);

Fix #14334: Support more msbuild conditional constructs #8039

Are you sure you want to change the base?

Fix #14334: Support more msbuild conditional constructs #8039

Conversation

autoantwort commented Dec 16, 2025

Uh oh!

Uh oh!

firewave commented Dec 16, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autoantwort commented Dec 17, 2025

Uh oh!

firewave commented Dec 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

autoantwort commented Jan 4, 2026

Uh oh!

sonarqubecloud bot commented Jan 8, 2026

Quality Gate passed

Uh oh!

autoantwort commented Jan 8, 2026

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autoantwort commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants